feat(cli): auto detect resolution #158
Conversation
The `present` command will now read by default the resolution of each presentation, and only change it if specified by the user. This PR also fixes bugs introduced by #156 and previous PRs, where the transition between two presentation was not correct...
jeertmans
added
bug
|
Hello @Fairlight8, let me explain how this PR implements a similar feature as reading the default background color (here, the default resolution), and apply changes (see comments). |
| class PresentationConfig(BaseModel): # type: ignore | ||
| slides: List[SlideConfig] | ||
| files: List[FilePath] | ||
| resolution: Tuple[PositiveInt, PositiveInt] = (1920, 1080) |
There was a problem hiding this comment.
@Fairlight8 first, I added a new resolution field. Specifying a default value is very important for backward compatibility (i.e., what to do when no resolution was specified? Use the default!).
| @property | ||
| def resolution(self) -> Tuple[int, int]: | ||
| """Returns the resolution.""" | ||
| return self.config.resolution | ||
|
|
There was a problem hiding this comment.
@Fairlight8 Second, add a property to Presentation so that Presentation.resolution maps to PresentationConfig.resolution
| @Slot() | ||
| def update_canvas(self) -> None: | ||
| """Update the canvas when a presentation has changed.""" | ||
| logger.debug("Updating canvas") | ||
| self.display_width, self.display_height = self.thread.current_resolution | ||
| if not self.isFullScreen(): | ||
| self.resize(self.display_width, self.display_height) |
There was a problem hiding this comment.
@Fairlight8 Third, every time the presentation changes, call to resize.
There, you will need to call setStyleSheet(...).
| default=(1920, 1080), | ||
| default=None, |
There was a problem hiding this comment.
@Fairlight8 Fourth, set the default to None
| resolution: Tuple[int, int], | ||
| resolution: Optional[Tuple[int, int]], |
There was a problem hiding this comment.
@Fairlight8 same as above, but for type hint
manim_slides/present.py
Outdated
| if resolution: | ||
| for presentation_config in presentation_configs: | ||
| presentation_config.resolution = resolution | ||
|
|
There was a problem hiding this comment.
@Fairlight8 Fifth, update the configs if resolution is not None*
*Previously it was if resolution:, but testing if not None is better (because some values are also fasly, e.g., 0).
| @property | ||
| def __resolution(self) -> Tuple[int, int]: | ||
| """Returns the scene's resolution used during rendering.""" | ||
| if MANIMGL: | ||
| return self.camera_config["pixel_width"], self.camera_config["pixel_height"] | ||
| else: | ||
| return config["pixel_width"], config["pixel_height"] | ||
|
|
There was a problem hiding this comment.
@Fairlight8 Sixth, create a new slide property that tells the resolution
There was a problem hiding this comment.
I don't get why you have to add yet another property here.
There was a problem hiding this comment.
You don’t « have to ». But as the definition depends on wether Manim or ManimGL is used, I prefer to hide the code behind a property :)
There was a problem hiding this comment.
Oh, okay, I understand now. No worries, that was silly, I thought we still were in present.py. I understand now.
| PresentationConfig(slides=self.__slides, files=files).json(indent=2) | ||
| PresentationConfig( | ||
| slides=self.__slides, files=files, resolution=self.__resolution | ||
| ).json(indent=2) |
There was a problem hiding this comment.
@Fairlight8 and finally add it to the constructor call :-)
|
Okay, I think I get it! You can push it, I will try to repeat the pattern for the background color. |
|
Nice, thanks @Fairlight8 ! |
The
presentcommand will now read by default the resolution of each presentation, and only change it if specified by the user.This PR also fixes bugs introduced by #156 and previous PRs, where the transition between two presentation was not correct...